Drop importances from khiops sklearn estimator attributes#494
Conversation
- separate construction rules into: - rules applied by default (`DEFAULT_CONSTRUCTION_RULES`); - calendar-related rules (`CALENDRICAL_CONSTRUCTION_RULES`); - document the construction rules; - fix the `construction_rules` parameter documentation in the Core API; - fix the `n_features` parameter documentation the Sklearn estimator API.
- the evaluated features characterize the analysis process, not the model itself - the used features characterize the model, but: - they are easily accessible through the `model_report_` attribute - they are not expected by the Scikit-learn ecosystem.
|
It is a bit confusing to mix the removal of the sklearn estimator attributes and the clarification of the variable construction rules in the same PR |
| "GetDate", | ||
| "GetTime", | ||
| # Construction rules | ||
| DEFAULT_CONSTRUCTION_RULES = [ |
There was a problem hiding this comment.
I am surprised that the default construct rules do not include the "calendrical" ones because legacy tools based on khiops seems to assume that.
Is this a change introduced with v11 ?
There was a problem hiding this comment.
Opening the Khiops 11.0.0-b.0 GUI, then "Parameters" > "Advanced predictor parameters" > "Variable construction parameters" shows which rules are enabled by default and which are not: we can see that multi-table rules (Entity and Table) are enabled by default, and that "calendrical" rules are not. In this PR we lift these to the Python API.
tramora
left a comment
tramora
left a comment
There was a problem hiding this comment.
My couple of remarks is not blocking
These updates are recycled from PR #486 which is no longer retained as per recent discussions. In the context of that PR, the link was that, in order for the importances to be relevant, we needed to make sure all rules are disabled, and having monotable datasets indeed disables all the rules by default since the calendrical rules are not applied unless requested by the user. In the context of the new PR, the juxtaposition seems artificial, but the idea is to opportunistically recycle the parts of the former PR #486 that are still retained. |
| comments, and dictionary and variable block internal comments. | ||
| - (`core`) Dictionary `Rule` class and supporting API for serializing `Rule` instances. | ||
| - (`core`) New way to add a variable to a dictionary using a complete specification. | ||
| - (`core`) New API constants for rules used in automatic variable construction: |
There was a problem hiding this comment.
Add the removed attributes in the Removed section.
There was a problem hiding this comment.
Ok, right. They had been added in 10.2.2.4.
popescu-v
force-pushed
the
drop-importances-from-khiops-sklearn-estimator-attributes
branch
from
bacd5c1 to
dc8bbd9
Compare
popescu-v
deleted the
drop-importances-from-khiops-sklearn-estimator-attributes
branch
3 participants
For a rationale, see the discussion at #480, namely #480 (comment) and #480 (comment).
TODO Before Asking for a Review
dev(ormainfor release PRs)Unreleasedsection ofCHANGELOG.md(no date)index.html